Skip to content

Add non-symlink case to bin/http_test_server #49

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 30, 2020
Merged

Add non-symlink case to bin/http_test_server #49

merged 1 commit into from
Sep 30, 2020

Conversation

TimWolla
Copy link
Contributor

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Related tickets #46, #47
Documentation n/a
License MIT

What's in this PR?

This PR hopefully adds direct support for running bin/http_test_server on Windows. Details are in #46.

I tested this PR by removing the other conditions and then running the script on Ubuntu. It worked fine. I did not actually test this on Windows, but I am confident that it should work properly. In any case we are not worse off compared to the situation before.

Example Usage

n/a

Checklist

  • Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix
  • Documentation pull request created (if not simply a bugfix)

To Do

  • Actually test on Windows (or just hope for the best).

@Nyholm
Copy link
Member

Nyholm commented Sep 30, 2020

Is this ready to merge?

@TimWolla
Copy link
Contributor Author

I don't plan any more changes from my side.

@Nyholm
Copy link
Member

Nyholm commented Sep 30, 2020

If you are “pretty sure” it works, the it is good enough for me.
:)

@Nyholm Nyholm merged commit 80205fa into php-http:master Sep 30, 2020
@TimWolla TimWolla deleted the bin-no-symlink branch September 30, 2020 18:49
@Nyholm
Copy link
Member

Nyholm commented Oct 1, 2020

Hm.. I tested on Guzzle and it does not work properly... Maybe Im doing something wrong: guzzle/guzzle#2790

@TimWolla
Copy link
Contributor Author

TimWolla commented Oct 1, 2020

It looks like you did everything correctly. I'll try in my personal fork later to see where it's still going wrong, because I don't want to leave loose ends here. Thanks.

@TimWolla
Copy link
Contributor Author

TimWolla commented Oct 1, 2020

For the record: I found the issue in @Nyholm's PR and added a review with a suggested change to his PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants